Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluetooth: Controller: Enable ISO support in the SDC #12508

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

rugeGerritsen
Copy link
Contributor

This commit enables the experimental ISO support in the SoftDevice Controller. See the changelog for more details.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 3, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 3, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-rs X
test-fw-nrfconnect-zigbee X
test-sdk-find-my X
test-sdk-homekit X
test-sdk-sidewalk X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI


#if defined(CONFIG_BT_CTLR_SYNC_ISO)
#define SDC_MEM_BIS_SINK \
SDC_MEM_PER_BIG(CONFIG_BT_CTLR_SYNC_ISO_STREAM_COUNT) + \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently trying to merge a fix to change this to CONFIG_BT_CTLR_SCAN_SYNC_ISO_SET

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

(sdc_hci_cmd_le_remove_iso_data_path_t const *)cmd_params,
(sdc_hci_cmd_le_remove_iso_data_path_return_t *)event_out_params);

case SDC_HCI_OPCODE_CMD_LE_ISO_TEST_END:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do that in a separate PR, but should we consider ifdef-ing out the test commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. We could consider adding more ifdefs for both the ISO test commands and also the Create CIG/BIG Test commands, Read Link quality and others

(sdc_hci_cmd_le_iso_test_end_t const *)cmd_params,
(sdc_hci_cmd_le_iso_test_end_return_t *)event_out_params);

case SDC_HCI_OPCODE_CMD_LE_SET_HOST_FEATURE:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in a CONFIG_BT_CTLR_SET_HOST_FEATURE ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -40,6 +40,7 @@ static bool command_generates_command_complete_event(uint16_t hci_opcode)
{
switch (hci_opcode) {
case SDC_HCI_OPCODE_CMD_LC_DISCONNECT:
case SDC_HCI_OPCODE_CMD_CB_WRITE_CONN_ACCEPT_TIMEOUT:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDC_HCI_OPCODE_CMD_CB_WRITE_CONN_ACCEPT_TIMEOUT creates a command complete

Copy link
Contributor

@Tschet1 Tschet1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

(sdc_hci_cmd_le_iso_test_end_t const *)cmd_params,
(sdc_hci_cmd_le_iso_test_end_return_t *)event_out_params);

case SDC_HCI_OPCODE_CMD_LE_SET_HOST_FEATURE:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@rtreccani rtreccani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

#if defined(CONFIG_BT_CTLR_ISO_TX_BUFFERS)
static int iso_handle(struct net_buf *acl)
{
LOG_DBG("");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of this?

@rugeGerritsen rugeGerritsen force-pushed the enable_iso_support branch 2 times, most recently from cd4866f to a8ee792 Compare October 4, 2023 11:27
This commit enables the experimental ISO support in the SoftDevice
Controller. See the changelog for more details.

Signed-off-by: Rubin Gerritsen <[email protected]>
@rugeGerritsen rugeGerritsen removed DNM changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Oct 5, 2023
@nordicjm nordicjm merged commit b37c09b into nrfconnect:main Oct 5, 2023
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants